-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[SDK Release] (Python) CQA inference 2025-05-15-preview #42870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
/azp run python - cognitivelanguage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
After the move to TypeSpec, Authoring and Inference API would be in separate artifacts. There shouldn't be any |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
|
@microsoft-github-policy-service agree company="Microsoft" |
|
/azp run python - cognitivelanguage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - cognitivelanguage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…ytypes score The verifytypes pipeline failed because type completeness for azure-ai-language-questionanswering dropped from 100% (main) to 87.9%. Pyright reported missing / ambiguous types for: QuestionAnsweringClient.question_answering (sync + async clients) QuestionAnsweringOperations.init (sync + async) Derived patch classes were then flagged as having partially unknown bases. Changes: Added explicit return/attribute annotation for question_answering in _client.py and _client.py. Added *args: Any, **kwargs: Any plus explicit internal attribute types in _operations.py and _operations.py. Result: Eliminates Pyright errors about missing annotations and partially unknown base classes. Restores type completeness score to parity with main (expected 100%). No runtime or public API behavior changes; typing-only improvement. Follow-up (optional): Consider templating these annotations in the code generator to avoid future regeneration loss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets check how we are generating code. The generated client does not look correct
sdk/cognitivelanguage/azure-ai-language-questionanswering/README.md
Outdated
Show resolved
Hide resolved
sdk/cognitivelanguage/azure-ai-language-questionanswering/README.md
Outdated
Show resolved
Hide resolved
sdk/cognitivelanguage/azure-ai-language-questionanswering/README.md
Outdated
Show resolved
Hide resolved
sdk/cognitivelanguage/azure-ai-language-questionanswering/README.md
Outdated
Show resolved
Hide resolved
sdk/cognitivelanguage/azure-ai-language-questionanswering/assets.json.bak
Outdated
Show resolved
Hide resolved
...nguage/azure-ai-language-questionanswering/azure/ai/language/questionanswering/aio/_patch.py
Outdated
Show resolved
Hide resolved
sdk/cognitivelanguage/azure-ai-language-questionanswering/migration_guide.md
Show resolved
Hide resolved
...ests/recordings/test_query_knowledgebase.pyTestQnAKnowledgeBasetest_query_knowledgebase.json
Outdated
Show resolved
Hide resolved
sdk/cognitivelanguage/azure-ai-language-questionanswering/tsp-location.yaml
Outdated
Show resolved
Hide resolved
...zure-ai-language-questionanswering/samples/async_samples/sample_query_knowledgebase_async.py
Outdated
Show resolved
Hide resolved
sdk/cognitivelanguage/azure-ai-language-questionanswering/tsp-location.yaml
Outdated
Show resolved
Hide resolved
|
/azp run python - cognitivelanguage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…location.yaml Co-authored-by: Rajarshi Sarkar <[email protected]>
| * Authoring functionality (project creation, knowledge source management, deployment operations) has been removed from this package and moved to a separate dedicated authoring package / namespace. All references to `AuthoringClient`, and related authoring operations have been eliminated from the runtime client distribution. | ||
| * Dropped support for Python versions earlier than 3.9 (aligns with Azure SDK Python support policy going forward). | ||
| * Model base class change: all public model types now inherit from `MutableMapping[str, Any]` (dict-like) instead of the previous `Model` base class. As a result they now support standard mutable mapping behavior (key iteration, item assignment, etc.) and any code depending on methods/properties inherited from the old base class should be reviewed/updated. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add that default_language has been removed, and give a link to how to customers can change the service default instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an actual guide/documentation that we can link to about how to configure this at the project level? Or in the portal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just check the service side, the default_language is hardcoded as "en",can not be configured at the project level, remove related line in changelog
| super().__init__( | ||
| question=question, text_documents=cast(List[TextDocument], text_documents), language=language, **kwargs | ||
| ) | ||
| self.string_index_type = "UnicodeCodePoint" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we're no longer providing this default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember it is not needed, but I'm not sure now. Add this back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make a note to confirm this before GA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is useful, add it back
| class MetadataFilter(_GeneratedMetadataFilter): | ||
| """Backward compatible MetadataFilter supporting legacy tuple/dict inputs.""" | ||
|
|
||
| def __init__(self, *args: Any, **kwargs: Any) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're missing the overloads for this model.
I'm also not sure if all the documentation will be inherited - so we should check how it renders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the overloads and update the documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're still missing the base overload that just supports a raw mapping? (You can find an example of this on all the generated models) - something like:
@overload
def __init__(self, mapping: Mapping[str, Any]) -> None:
...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete this patch file, it's false added
...ai-language-questionanswering/azure/ai/language/questionanswering/_operations/_operations.py
Show resolved
Hide resolved
...ai-language-questionanswering/azure/ai/language/questionanswering/_operations/_operations.py
Show resolved
Hide resolved
...itivelanguage/azure-ai-language-questionanswering/samples/async_samples/sample_chat_async.py
Show resolved
Hide resolved
|
@Amichelangelo I've opened a new pull request, #43778, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@Amichelangelo I've opened a new pull request, #43779, to work on those changes. Once the pull request is ready, I'll request review from you. |
…into qiyin/cqa-sdk-py
| from __future__ import annotations | ||
|
|
||
| from collections.abc import Iterable, Mapping, MutableMapping | ||
| from typing import Any, Optional, Sequence, Tuple, Union, Mapping as TypingMapping, List, overload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need separate imports of Mapping from typing vs collections.abc - they are aliases for the same object:
https://docs.python.org/3/library/typing.html#typing.Mapping
So you only need to import once from collections.abc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| example, use "en" for English; "es" for Spanish etc. If not set, use "en" for English as | ||
| default. | ||
| :vartype language: str | ||
| metadata: Sequence[Union[Tuple[str, str], MetadataRecord, TypingMapping[str, str]]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't support a union sequence like this - I don't think it's too much to ask that users provide a homogenous list. Keeps things a bit more simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, fix it
| class MetadataFilter(_GeneratedMetadataFilter): | ||
| """Backward compatible MetadataFilter supporting legacy tuple/dict inputs.""" | ||
|
|
||
| def __init__(self, *args: Any, **kwargs: Any) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're still missing the base overload that just supports a raw mapping? (You can find an example of this on all the generated models) - something like:
@overload
def __init__(self, mapping: Mapping[str, Any]) -> None:
...| super().__init__( | ||
| question=question, text_documents=cast(List[TextDocument], text_documents), language=language, **kwargs | ||
| ) | ||
| self.string_index_type = "UnicodeCodePoint" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make a note to confirm this before GA.
| self.string_index_type = "UnicodeCodePoint" | ||
| # _attribute_map may not exist if underlying generator changed; guard accordingly. | ||
| try: # pragma: no cover - defensive | ||
| self._attribute_map.update({"string_index_type": {"key": "stringIndexType", "type": "str"}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than do this, shouldn't we do something like:
def __init__(
self,
*,
question: str,
text_documents: Sequence[Union[str, TextDocument]],
language: Optional[str] = None,
**kwargs: Any,
) -> None:
string_index_type = kwargs.pop("string_index_type", "UnicodeCodePoint")
super().__init__(question=question, text_documents=normalized, language=language, string_index_type=string_index_type)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from the changelog, the string_index_type is hardcoded to Utf16CodeUnit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to use kwargs.pop("string_index_type", "UnicodeCodePoint"), it's designed to use hardcode UnicodeCodePoint
|
/azp run python - cognitivelanguage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - cognitivelanguage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
If an SDK is being regenerated based on a new API spec, a link to the pull request containing these API spec changes should be included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines